Skip to content

[SPARK-32106][SQL] Implement script transform in sql/core#29414

Closed
AngersZhuuuu wants to merge 16 commits into
apache:masterfrom
AngersZhuuuu:SPARK-32106-MINOR
Closed

[SPARK-32106][SQL] Implement script transform in sql/core#29414
AngersZhuuuu wants to merge 16 commits into
apache:masterfrom
AngersZhuuuu:SPARK-32106-MINOR

Conversation

@AngersZhuuuu
Copy link
Copy Markdown
Contributor

@AngersZhuuuu AngersZhuuuu commented Aug 12, 2020 •
edited
Loading

What changes were proposed in this pull request?

  • Implement SparkScriptTransformationExec based on BaseScriptTransformationExec
  • Implement SparkScriptTransformationWriterThread based on BaseScriptTransformationWriterThread of writing data
  • Add rule SparkScripts to support convert script LogicalPlan to SparkPlan in Spark SQL (without hive mode)
  • Add SparkScriptTransformationSuite test spark spec case
  • add test in SQLQueryTestSuite

And we will close #29085 .

Why are the changes needed?

Support user use Script Transform without Hive

Does this PR introduce any user-facing change?

User can use Script Transformation without hive in no serde mode.
Such as :
**default no serde **

SELECT TRANSFORM(a, b, c)
USING 'cat' AS (a int, b string, c long)
FROM testData

no serde with spec ROW FORMAT DELIMITED

SELECT TRANSFORM(a, b, c)
ROW FORMAT DELIMITED
FIELDS TERMINATED BY '\t'
COLLECTION ITEMS TERMINATED BY '\u0002'
MAP KEYS TERMINATED BY '\u0003'
LINES TERMINATED BY '\n'
NULL DEFINED AS 'null'
USING 'cat' AS (a, b, c)
ROW FORMAT DELIMITED
FIELDS TERMINATED BY '\t'
COLLECTION ITEMS TERMINATED BY '\u0004'
MAP KEYS TERMINATED BY '\u0005'
LINES TERMINATED BY '\n'
NULL DEFINED AS 'NULL'
FROM testData

How was this patch tested?

Added UT

@AngersZhuuuu
Copy link
Copy Markdown
Contributor Author

AngersZhuuuu commented Aug 12, 2020

FYI @maropu @cloud-fan

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 12, 2020

Test build #127370 has finished for PR 29414 at commit b4c4b40.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class SparkScriptTransformationExec(
  • case class SparkScriptTransformationWriterThread(

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 12, 2020

Test build #127373 has finished for PR 29414 at commit c58729b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

maropu
maropu reviewed Aug 13, 2020
View reviewed changes
Comment thread sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala
Comment thread sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala
Comment thread sql/core/src/main/scala/org/apache/spark/sql/execution/SparkScriptTransformationExec.scala
Comment thread sql/core/src/test/resources/sql-tests/inputs/transform.sql Outdated
maropu
maropu reviewed Aug 13, 2020
View reviewed changes
Comment thread sql/core/src/main/scala/org/apache/spark/sql/execution/SparkScriptTransformationExec.scala Outdated
maropu
maropu reviewed Aug 13, 2020
View reviewed changes
Comment thread sql/core/src/main/scala/org/apache/spark/sql/execution/SparkScriptTransformationExec.scala Outdated
maropu
maropu reviewed Aug 13, 2020
View reviewed changes
Comment thread sql/core/src/test/resources/sql-tests/results/transform.sql.out
FIELDS TERMINATED BY '|'
LINES TERMINATED BY '\n'
NULL DEFINED AS 'NULL'
USING 'cat' AS (a, b, c, d)
Copy link
Copy Markdown
Member

@maropu maropu Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can ROW FORMAT DELIMETED with output schema work correctly?

Copy link
Copy Markdown
Member

@maropu maropu Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, could you add test cases for the parser, too? https://github.com/apache/spark/pull/29414/files#diff-36e2b29ae675caaa1fce16e74fbd8710R1135

Copy link
Copy Markdown
Contributor Author

@AngersZhuuuu AngersZhuuuu Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maropu

see two more bug founded
https://issues.apache.org/jira/browse/SPARK-32607
https://issues.apache.org/jira/browse/SPARK-32608

Copy link
Copy Markdown
Contributor Author

@AngersZhuuuu AngersZhuuuu Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, could you add test cases for the parser, too? https://github.com/apache/spark/pull/29414/files#diff-36e2b29ae675caaa1fce16e74fbd8710R1135

Add some UT in transform.sql and fix origin wrong part

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 13, 2020

Test build #127402 has finished for PR 29414 at commit fc5bbce.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 13, 2020

Test build #127406 has finished for PR 29414 at commit fdfc987.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AngersZhuuuu AngersZhuuuu mentioned this pull request Aug 16, 2020
Closed
@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 24, 2020

Test build #127829 has finished for PR 29414 at commit dabae9b.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu
Copy link
Copy Markdown
Member

maropu commented Aug 24, 2020

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 24, 2020

Test build #127834 has finished for PR 29414 at commit dabae9b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AngersZhuuuu AngersZhuuuu changed the title [SPARK-32106][SQL] Implement script transform in sql/core [SPARK-32106][SQL][test-hadoop2.7][test-hive1.2] Implement script transform in sql/core Aug 24, 2020
@AngersZhuuuu
Copy link
Copy Markdown
Contributor Author

AngersZhuuuu commented Aug 24, 2020

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 24, 2020

Test build #127841 has finished for PR 29414 at commit dabae9b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AngersZhuuuu
Copy link
Copy Markdown
Contributor Author

AngersZhuuuu commented Aug 30, 2020

ping @maropu @HyukjinKwon @cloud-fan

@AngersZhuuuu
Copy link
Copy Markdown
Contributor Author

AngersZhuuuu commented Sep 2, 2020

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Sep 2, 2020

Test build #128204 has finished for PR 29414 at commit dabae9b.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AngersZhuuuu
Copy link
Copy Markdown
Contributor Author

AngersZhuuuu commented Sep 2, 2020

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Sep 2, 2020

Test build #128205 has finished for PR 29414 at commit dabae9b.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AngersZhuuuu
Copy link
Copy Markdown
Contributor Author

AngersZhuuuu commented Dec 14, 2020

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 14, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37371/

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 14, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37371/

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 14, 2020

Test build #132769 has finished for PR 29414 at commit d3c65d4.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Copy Markdown
Member

HyukjinKwon commented Dec 15, 2020

I am good with this change. I don't mind if somebody merges.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 15, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37396/

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 15, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37396/

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 15, 2020

Test build #132794 has finished for PR 29414 at commit a10c5a6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AngersZhuuuu
Copy link
Copy Markdown
Contributor Author

AngersZhuuuu commented Dec 15, 2020

Hope merge this and start next work

@AngersZhuuuu
Copy link
Copy Markdown
Contributor Author

AngersZhuuuu commented Dec 16, 2020

gentle ping @maropu @cloud-fan

@alfozan
Copy link
Copy Markdown

alfozan commented Dec 17, 2020

gentle ping @tejasapatil @sameeragarwal

@maropu
Copy link
Copy Markdown
Member

maropu commented Dec 17, 2020

okay, I will merge this into master in a few days if nobody has more comments.

@maropu
Copy link
Copy Markdown
Member

maropu commented Dec 18, 2020

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 18, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37574/

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 18, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37574/

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 18, 2020

Test build #132973 has finished for PR 29414 at commit a10c5a6.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu
Copy link
Copy Markdown
Member

maropu commented Dec 18, 2020

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 18, 2020

Test build #132972 has finished for PR 29414 at commit a10c5a6.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 18, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37581/

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 18, 2020

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37581/

@SparkQA
Copy link
Copy Markdown

SparkQA commented Dec 18, 2020

Test build #132980 has finished for PR 29414 at commit a10c5a6.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu
Copy link
Copy Markdown
Member

maropu commented Dec 22, 2020

okay, I believe we have much time to revisit this until the Spark v3.2.0 released, so I'll merge this into master for now so as to encourage @AngersZhuuuu 's following work. FYI: @HyukjinKwon @cloud-fan @dongjoon-hyun @gatorsmile

@maropu maropu closed this in 7466031 Dec 22, 2020
@maropu
Copy link
Copy Markdown
Member

maropu commented Dec 22, 2020

Merged to master.

@HyukjinKwon
Copy link
Copy Markdown
Member

HyukjinKwon commented Dec 22, 2020

Nice, I support your decision @maropu.

@dongjoon-hyun
Copy link
Copy Markdown
Member

dongjoon-hyun commented Dec 22, 2020

Thank you for informing, @maropu !

dongjoon-hyun
dongjoon-hyun reviewed Dec 22, 2020
View reviewed changes
Comment thread sql/core/src/test/resources/sql-tests/results/transform.sql.out
struct<>
-- !query output
org.apache.spark.SparkException
Subprocess exited with status 127. Error: /bin/bash: some_non_existent_command: command not found
Copy link
Copy Markdown
Member

@dongjoon-hyun dongjoon-hyun Dec 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to cause a test flakiness in GitHub Action. Could you take a look, @AngersZhuuuu and @maropu .

SQLQueryTestSuite.transform.sql
org.scalatest.exceptions.TestFailedException: transform.sql
Expected "...istent_command: comm[and not found]", but got "...istent_command: comm[]" Result did not match for query #2
SELECT TRANSFORM(a)
USING 'some_non_existent_command' AS (a)
FROM t

Copy link
Copy Markdown
Member

@dongjoon-hyun dongjoon-hyun Dec 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very flaky. Almost 50% failure probability.

Copy link
Copy Markdown
Member

@maropu maropu Dec 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's too bad. Thanks for letting me know. I'll open a followup PR to fix it.

Copy link
Copy Markdown
Member

@dongjoon-hyun dongjoon-hyun Dec 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for taking a look at that, @maropu .

Copy link
Copy Markdown
Member

@maropu maropu Dec 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review this: #30896

Copy link
Copy Markdown
Contributor Author

@AngersZhuuuu AngersZhuuuu Dec 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review this: #30896

Thank you, @maropu
Yesterday I also found some different between UT and transform.sql. USING 'cat 1>&2' works well in SQLQuerySuite but won't work in transform.sql

@xuanyuanking xuanyuanking mentioned this pull request Jan 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maropu maropu maropu approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@AngersZhuuuu @SparkQA @maropu @HyukjinKwon @alfozan @dongjoon-hyun